Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add minimal c++ example and check #1944

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aidenfoxivey
Copy link

@aidenfoxivey aidenfoxivey commented Oct 2, 2024

Adding a simple C++ test to CI.

Fixes #1770.

  • Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
    Doesn't change any cryptographic algorithms.

  • Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider will also need to be ready for review and merge by the time this is merged.)
    Doesn't change any cryptographic algorithms.

Signed-off-by: Aiden Fox Ivey <[email protected]>
@aidenfoxivey aidenfoxivey changed the title Add minimal c++ and check Add minimal c++ example and check Oct 2, 2024
@vsoftco
Copy link
Member

vsoftco commented Oct 2, 2024

@aidenfoxivey We have a header-only C++ wrapper that provides all the required C++ RAII for liboqs, https://github.com/open-quantum-safe/liboqs-cpp

@vsoftco vsoftco self-assigned this Oct 2, 2024
@vsoftco vsoftco added the wontfix This will not be worked on label Oct 2, 2024
@aidenfoxivey
Copy link
Author

@vsoftco Would it be fair to rework this with the C++ bindings and then continue working on the PR?

@aidenfoxivey
Copy link
Author

Absolutely fine to close the PR too, but I feel like it should probably be addressed in the issue first, just to make the procedure clear.

@Martyrshot
Copy link
Member

Martyrshot commented Oct 2, 2024

@vsoftco The purpose of this is to catch syntax errors in C++. For example when the stateful-sigs PR was in progress we almost merged something that compiled for C but failed to link against a C++ target.

Since we have #ifdef CPLUSPLUS blocks in our headers I think this check needs to be done with our standard C headers so we catch issues before merging things into liboqs main rather than waiting for the C++ wrappers to fail

@Martyrshot Martyrshot removed the wontfix This will not be worked on label Oct 2, 2024
@Martyrshot Martyrshot self-assigned this Oct 2, 2024
@aidenfoxivey
Copy link
Author

@Martyrshot Do we want to compile and run the resulting example file? (and/or use valgrind to check it for leaks?) in my existing implementation I'm just compiling and linking it.

Signed-off-by: Aiden Fox Ivey <[email protected]>
@Martyrshot
Copy link
Member

I think valgrind for this is overkill, but running it is a good idea. It could catch weird runtime linking errors. Sorry I should have thought about this sooner. Would it be much work for you to add this test to the ninja run_tests rule? Then it would always be run rather then having a specific ci job.

If it's too much work, I can take a look into doing this later in the week.

@aidenfoxivey
Copy link
Author

Oh no, don't worry about it! I can absolutely sort it out later tonight or tomorrow.

@aidenfoxivey
Copy link
Author

@Martyrshot I thought about it a bit and, if I understand you correctly, this requires modifying the CMakeLists.txt to add C++ as a LANGUAGE. Are you cool with that?

@Martyrshot
Copy link
Member

Martyrshot commented Oct 3, 2024

Would we be able to isolate that change to just this test?

@aidenfoxivey
Copy link
Author

The ninja run-tests is generated based on CMakeLists.txt right? I think in that case, you pretty much have to change the CMake infrastructure to allow it to build whenever you call ninja run-tests. The reason I didn't have to earlier was because I was just hardcoding a call to g++ in the Github action.

@Martyrshot
Copy link
Member

That's fair. Then in that case I think we should avoid modifying CMakeLists.txt. Let's relocate the .cpp file you created into the .github folder somewhere so that it's not mistaken as a core liboqs test. Then have the c++ action run the test and enforce that it doesn't error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add C++ test to ci
3 participants